:seedling: Preserve Mozilla v5.8 old TLS profile and harden update script by tmshort · Pull Request #2632 · operator-framework/operator-controller

tmshort · 2026-04-07T15:45:59Z

Move oldTLSProfile to a static old_profile.go (removed from v6+ spec)
Add version-based early exit to update-tls-profiles.sh
Add profile existence and tls_versions field validation in script
Add unit tests for old profile content and X25519MLKEM768 curve

Description

Reviewer Checklist

API Go Documentation
Tests: Unit Tests (and E2E Tests, if appropriate)
Comprehensive Commit Messages
Links to related GitHub Issue(s)

netlify · 2026-04-07T15:46:06Z

✅ Deploy Preview for olmv1 ready!

Name	Link
🔨 Latest commit	`4340f84`
🔍 Latest deploy log	https://app.netlify.com/projects/olmv1/deploys/69d5403237c43c000831370d
😎 Deploy Preview	https://deploy-preview-2632--olmv1.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

Copilot

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Preserves the deprecated Mozilla v5.8 “old” TLS profile as a static Go source while updating the generator to track Mozilla’s latest TLS guidelines and adding guards/tests to prevent accidental regressions.

Changes:

Move oldTLSProfile into a new static old_profile.go (sourced from Mozilla v5.8) and remove it from generated data.
Harden update-tls-profiles.sh with version-based early exit and schema validations; switch input to latest.json / v6.
Add unit tests for old profile invariants and for presence of the X25519MLKEM768 curve across profiles.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.

File	Description
internal/shared/util/tlsprofiles/tlsprofiles_test.go	Adds assertions to lock down old profile invariants and ensure `X25519MLKEM768` is present in profiles.
internal/shared/util/tlsprofiles/old_profile.go	Introduces a static copy of Mozilla v5.8 “old” profile for backward compatibility.
internal/shared/util/tlsprofiles/mozilla_data.go	Updates generated header to track `latest.json` (v6) and removes generated “old” profile.
hack/tools/update-tls-profiles.sh	Switches generator input to `latest.json`, adds early exit on unchanged version, and validates input structure before generation.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

internal/shared/util/tlsprofiles/tlsprofiles_test.go

hack/tools/update-tls-profiles.sh

internal/shared/util/tlsprofiles/tlsprofiles_test.go

hack/tools/update-tls-profiles.sh

internal/shared/util/tlsprofiles/tlsprofiles_test.go

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

hack/tools/update-tls-profiles.sh

codecov · 2026-04-07T16:44:34Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 68.95%. Comparing base (4510b1b) to head (4340f84).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2632   +/-   ##
=======================================
  Coverage   68.95%   68.95%           
=======================================
  Files         139      139           
  Lines        9891     9891           
=======================================
  Hits         6820     6820           
  Misses       2562     2562           
  Partials      509      509

Flag	Coverage Δ
e2e	`37.27% <ø> (-0.29%)`	⬇️
experimental-e2e	`52.25% <ø> (ø)`
unit	`53.60% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

- Move oldTLSProfile to a static old_profile.go (removed from v6+ spec) - Add version-based early exit to update-tls-profiles.sh - Validate profile existence, tls_versions, ciphers, and curves fields - Make jq/sed/cat invocations null-safe and consistently quote variables - Add unit tests for old profile content; fix global state leak in tests Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Todd Short <tshort@redhat.com>

fgiudici

/lgtm

fgiudici · 2026-04-08T08:29:16Z

hack/tools/update-tls-profiles.sh


-version=$(${JQ} -r '.version' ${TMPFILE})
+# Extract stored version from current output file (may be empty on first run)
+STORED_VERSION=$(grep '^// DATA VERSION:' "${OUTPUT}" 2>/dev/null | awk '{print $4}' || true)


nit: the "|| true" may be dropped here as the grep is in a pipe with awk and the overall result would never fail (as the awk command would not fail). Still looks more robust this way.

pedjak

1. Use a fixed version URL instead of `latest.json`

Using latest.json makes builds non-reproducible — the same commit can pass CI today and break tomorrow if Mozilla publishes a new version (which is exactly what triggered this PR). A fixed URL like guidelines/6.0.json makes version bumps deliberate, reviewable, and auditable. It also makes the version-skip logic unnecessary since the input becomes deterministic.

2. Consider replacing bash/jq/sed code generation with `go:embed` + runtime parsing

The script has grown significantly with validation, null-safety, and sed workarounds. An alternative approach: commit the Mozilla JSON file directly, embed it with //go:embed, and parse it at init() time using the existing cipherSuiteId()/curveId() helpers. This eliminates the bash script, the gojq build dependency, the sed hacks for unsupported ciphers (skip unknown names during parsing instead), and the generated mozilla_data.go. The make target becomes a one-line curl.

3. The old profile's curves don't match v5.8

The static old_profile.go includes X25519MLKEM768 in its curves, but Mozilla v5.8 didn't have that curve — it was added in v6. If the intent is to preserve v5.8 faithfully, the curves should be X25519, prime256v1, secp384r1.

4. The `sed` cleanup is now dead code

The sed lines removing TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA384 etc. only applied to the old profile, which is no longer generated. These can be removed.

5. Minor: `TestGetProfiles` negative case dropped

The original test covered the "does-not-exist" error path. The replacement TestProfilesMapCompleteness only tests the happy path. Consider keeping the negative case.

tmshort · 2026-04-08T13:17:08Z

1. Use a fixed version URL instead of latest.json

This is intentional; because we want to ensure we are up-to-date.
We explicitly chose to use latest.json so that we know about changes.
The previous PR in here changed it from latest.json to 5.8.json; this restores it.

2. Consider replacing bash/jq/sed code generation with go:embed + runtime parsing

Interesting idea.

3. The old profile's curves don't match v5.8

Actually, 5.8 does include X25519MLKEM768, from https://ssl-config.mozilla.org/guidelines/5.8.json

           "tls_curves": ["X25519MLKEM768", "X25519", "prime256v1", "secp384r1"],

It wasn't part of the original 5.7.

4. The sed cleanup is now dead code

If those ciphers are ever added to the intermediate (possible) or the modern (unlikely) profile. They will cause problems. So even though they previously only applied to the to the old profile, they could apply to any other profile. If this were moved to use golang, then all ciphers could be validated, and not just those that we list.

5. Minor: TestGetProfiles negative case dropped

👍

grokspawn · 2026-04-08T13:24:49Z

+1 to approve/merge, but I'm not the only one with comments, so I won't add the label here yet.

openshift-ci · 2026-04-08T13:28:57Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: camilamacedo86

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [camilamacedo86]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

camilamacedo86 · 2026-04-08T13:29:53Z

/hold

Due:
+1 to approve/merge, but I'm not the only one with comments, so I won't add the label here yet.

@pedjak are you ok with?
@tmshort feel free to unhold when you see fit

tmshort · 2026-04-08T13:32:40Z

/unhold

Since @pedjak's number 2 comment is a big thing, I'm going to do a followup PR with it, and will resolve numbers 4 (which is harmless now) and 5 at the same time.

Number 1 is intentional, and 3 is incorrect.

Copilot AI review requested due to automatic review settings April 7, 2026 15:46

openshift-ci bot requested review from ankitathomas and bentito April 7, 2026 15:46

Copilot AI reviewed Apr 7, 2026

View reviewed changes

Copilot started reviewing on behalf of tmshort April 7, 2026 15:55 View session

grokspawn reviewed Apr 7, 2026

View reviewed changes

hack/tools/update-tls-profiles.sh Show resolved Hide resolved

grokspawn reviewed Apr 7, 2026

View reviewed changes

hack/tools/update-tls-profiles.sh Show resolved Hide resolved

tmshort force-pushed the tls-profiles-old-profile-preservation branch from fbafd47 to 2de30ac Compare April 7, 2026 16:09

grokspawn reviewed Apr 7, 2026

View reviewed changes

internal/shared/util/tlsprofiles/tlsprofiles_test.go Outdated Show resolved Hide resolved

Copilot AI review requested due to automatic review settings April 7, 2026 16:23

tmshort force-pushed the tls-profiles-old-profile-preservation branch from 2de30ac to b4a4e74 Compare April 7, 2026 16:23

Copilot started reviewing on behalf of tmshort April 7, 2026 16:23 View session

Copilot AI reviewed Apr 7, 2026

View reviewed changes

hack/tools/update-tls-profiles.sh Outdated Show resolved Hide resolved

hack/tools/update-tls-profiles.sh Outdated Show resolved Hide resolved

hack/tools/update-tls-profiles.sh Outdated Show resolved Hide resolved

hack/tools/update-tls-profiles.sh Outdated Show resolved Hide resolved

tmshort force-pushed the tls-profiles-old-profile-preservation branch from b4a4e74 to 4340f84 Compare April 7, 2026 17:34

fgiudici reviewed Apr 8, 2026

View reviewed changes

openshift-ci bot assigned fgiudici Apr 8, 2026

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Apr 8, 2026

pedjak reviewed Apr 8, 2026

View reviewed changes

camilamacedo86 approved these changes Apr 8, 2026

View reviewed changes

openshift-ci bot assigned camilamacedo86 Apr 8, 2026

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 8, 2026

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 8, 2026

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 8, 2026

openshift-merge-bot bot merged commit 8614e3b into operator-framework:main Apr 8, 2026
26 checks passed

tmshort mentioned this pull request Apr 8, 2026

🌱 Add negative profile lookup test case #2633

Open

4 tasks

Conversation

tmshort commented Apr 7, 2026

Description

Reviewer Checklist

Uh oh!

netlify bot commented Apr 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for olmv1 ready!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Apr 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

fgiudici left a comment

Choose a reason for hiding this comment

Uh oh!

fgiudici Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

pedjak left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

1. Use a fixed version URL instead of latest.json

2. Consider replacing bash/jq/sed code generation with go:embed + runtime parsing

3. The old profile's curves don't match v5.8

4. The sed cleanup is now dead code

5. Minor: TestGetProfiles negative case dropped

Uh oh!

tmshort commented Apr 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

1. Use a fixed version URL instead of latest.json

2. Consider replacing bash/jq/sed code generation with go:embed + runtime parsing

3. The old profile's curves don't match v5.8

4. The sed cleanup is now dead code

5. Minor: TestGetProfiles negative case dropped

Uh oh!

grokspawn commented Apr 8, 2026

Uh oh!

openshift-ci bot commented Apr 8, 2026

Uh oh!

camilamacedo86 commented Apr 8, 2026

Uh oh!

tmshort commented Apr 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

netlify bot commented Apr 7, 2026 •

edited

Loading

codecov bot commented Apr 7, 2026 •

edited

Loading

pedjak left a comment •

edited

Loading

1. Use a fixed version URL instead of `latest.json`

2. Consider replacing bash/jq/sed code generation with `go:embed` + runtime parsing

4. The `sed` cleanup is now dead code

5. Minor: `TestGetProfiles` negative case dropped

tmshort commented Apr 8, 2026 •

edited

Loading

1. Use a fixed version URL instead of `latest.json`

2. Consider replacing bash/jq/sed code generation with `go:embed` + runtime parsing

4. The `sed` cleanup is now dead code

5. Minor: `TestGetProfiles` negative case dropped

tmshort commented Apr 8, 2026 •

edited

Loading